feat: apply OEP-66 queryset-scoping pattern to Enrollments v2 API - #39037
Faraz32123 merged 3 commits into
Conversation
78d3f46 to
3678f4c
Compare
| def test_filter_by_username_narrows(self): | ||
| """User-driven filter (filter_queryset) still narrows by username.""" | ||
| self.client.force_authenticate(user=self.admin) | ||
| response = self.client.get(self.url, {"username": self.learner_b.username}) |
There was a problem hiding this comment.
This should only work for admins correct? we should have the inverted test validating that users can't see eachothers enrollments by passing eachothers names in.
There was a problem hiding this comment.
yup that should be admin only. Just added test_non_admin_cannot_list_another_users_enrollments for it
|
|
||
| def scope(self, queryset, subject): | ||
| # Platform admins (IsAdminUser) see all enrollments; nothing to narrow yet. | ||
| return queryset |
There was a problem hiding this comment.
This is a lot of boilerplate for essentially a no-op on scoping, can we simplify this? Maybe we have some sort of a "FullScopePolicy" that can be used in many places for things like this?
There was a problem hiding this comment.
I think FullScopePolicy would be a good to add since any admin only list that adopts the scoping layer would otherwise end up hand writing the same pass through class, and since its a reusable DRF tooling it should be placed next to ScopingPolicy / ScopedQuerysetMixin so plugins can use it too.
I've opened openedx/edx-drf-extensions#578 adding it there, and this PR now imports it from the library. CI here will stay red until that's released; once you approve, merge and publish 10.9.0, I'll bump the pin in this PR and it should be good to merge.
3678f4c to
b106b61
Compare
b106b61 to
3ca8acd
Compare
Review feedback on openedx#39037: the code should follow the standards without naming the documents. The three authoring urls.py modules now read "Authoring API vN URLs.", the URL-structure test banners and the Enrollment v2 module docstring lose their rule references, and the remaining comments say what the code does instead — deprecation window rather than OEP-21 window, "whose course_key path converter hands views a parsed key" rather than a rule number. Only lines this branch adds are touched; the pre-existing ADR references elsewhere in these files are left alone, the one exception being the ADR 0028 line in the Enrollment v2 module docstring, which this branch was already rewriting. Prose only apart from one assert message in the new URL-structure test, which now reads "missing error-envelope field". ruff passes.
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
Looks good, once this can be rebased on the published version of openedx/edx-drf-extensions#578
|
The drf-extensions PR has been merged and published, can you update this to pull that in? |
Wire EnrollmentsAdminListView through ScopedQuerysetMixin from edx-drf-extensions so endpoint access, record visibility and user-driven filtering are kept separate. Platform admins may see every enrollment, so the view uses the library's FullScopePolicy for now; a narrower policy can replace it later without touching the view. The form-based filtering moves from get_queryset() into filter_queryset(), so it only narrows the scoped queryset. No behavior change. Adds regression tests for the admin list: 401/403, a regular user filtering by someone else's username gets 403 with no data, admins see all rows, the course_key/username filters, the 400 on invalid params, and the ADR 0033 Deprecation header. Bumps edx-drf-extensions 10.8.0 -> 10.9.0, the release that adds FullScopePolicy (edx-drf-extensions#578). Follows up openedx#38847, whose shared scoping tooling moved to edx-drf-extensions in #569.
3ca8acd to
a153a0f
Compare
Resolves conflicts with openedx#39037 (queryset scoping) and the edx-drf-extensions 10.9.0 bump on master. Keeps both new Enrollment v2 test classes; takes master's requirements and uv.lock, which supersede this branch's 10.8.0 bump so the branch no longer changes any requirements file.
3 participants
Adopt the OEP-66 "Separating Authorization Concerns in List Endpoints" record-visibility layer on the admin enrollment list, using the shared building blocks now published in edx-drf-extensions.
EnrollmentsAdminListView is an ORM-backed ListAPIView, so it wires the three authorization concerns separately:
Bumps edx-drf-extensions 10.6.0 -> 10.7.0, the release that adds the reusable ScopingPolicy (a typing.Protocol) and ScopedQuerysetMixin. Adds TestEnrollmentsAdminListView regression tests covering endpoint access (401/403), pass-through scoping (admin sees all rows), the course_key/username filters, the 400-on-invalid-params path, the ADR 0033 Deprecation header, and the scoping-policy pass-through.
Follows up the closed PR #38847: per review, the shared ScopingPolicy / ScopedQuerysetMixin tooling moved to edx-drf-extensions (#569) as a subject-based typing.Protocol with a duck-typed mixin check, and this change consumes it rather than defining it locally.